Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GPU] set output from variable's memory if kv-cache #27658

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

sungeunk
Copy link
Contributor

Tickets:

  • 157514

@sungeunk sungeunk requested review from a team as code owners November 21, 2024 07:29
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Nov 21, 2024
@@ -618,6 +618,7 @@ void primitive_inst::realloc_if_needed() {
_max_output_layout_count[j] = 0;
}
} else {
_outputs[0] = variable.get_memory();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions about this PR:

  • This is setting _outputs[0] on realloc_if_needed. In case realloc does not happen, are we using proper buffer for _outputs[0]? I'm afraid this should be set before this function.
  • Does shape_predictor work correctly for multi-ireq scenario? From the code, it seems like that the shape_predictor is placed within the network and it may have issue for multi-ireq scenario as it does not receive ireq information. We may need to take ireq information as an argument for that. Could you comment on it? @sshlyapn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isanghao I think there shouldn't be any problems with shape_predictor, since each ireq has unique instance of shape_predictor (which is passed to the network after the network is assigned to ireq):

std::shared_ptr<cldnn::ShapePredictor> m_shape_predictor = nullptr;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you already did the work ;) Thanks for confirmation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sungeunk Could you please describe the issue in more detail? Does this mean that the KV-cache from one inference request is being used in another request in some cases? If so, then we probably need to adjust scales and zero points memory buffers as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sshlyapn Memory of variable-state in two infer-requests are shared at this sample code.minicpm_multi_inferreq.cpp
results of query_state() for llmIR are changed when it calls llmIR2.infer().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sungeunk , thank you for the details. In that case, it seems we have to update the scales/zero points buffers as well

@sungeunk sungeunk force-pushed the 157514_conflicts_inf_reqs branch from dda463c to 3efd529 Compare November 25, 2024 09:39
@sungeunk sungeunk requested a review from isanghao November 25, 2024 09:40
@sungeunk sungeunk requested review from isanghao and sshlyapn December 2, 2024 01:24
@isanghao isanghao added this pull request to the merge queue Dec 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 2, 2024
@isanghao isanghao added this pull request to the merge queue Dec 2, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 2, 2024
@isanghao isanghao added this pull request to the merge queue Dec 2, 2024
Merged via the queue into openvinotoolkit:master with commit 4a4bfed Dec 2, 2024
155 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants